Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[P4Testgen] Implement meter support for the BMv2 V1model PTF test back end #3974

Merged
merged 10 commits into from
May 2, 2023

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Apr 7, 2023

Implement P4Testgen BMv2 support for meters and direct meters.

  • Add an interface to the base_test.py file to configure meters.
  • Also add a helper function to the PTF test back end to make sure we are able to set the color we want. For green, we set the thresholds to the maximum, for red we set the thresholds to the minimum, for yellow we use a mix.
  • direct meters can only be used if they are attached to a table and the table has an entry. Implement a new pass that creates a direct meter <-> table mapping. Look up this mapping when executing the extern. If there is no table entry in the table associated with the meter, skip meter execution since we can not configure it.

if direct:
meter_write = entity.direct_meter_entry
meter_obj = self.get_obj("direct_meters", meter_name)
meter_write.table_entry.table_id = meter_obj.direct_table_id
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antoninbas I have a question about direct_meter writes. I am trying to use the p4runtime API to craft an update message, but I am unable to get this particular write to work. The message I am seeing is
* At index 0: INVALID_ARGUMENT, 'Missing non-ternary field in match key'.

I am not exactly sure how to craft a match key here that makes sense. I tried various things but keep getting the same message. Have not found a similar example, other operations on direct externs seem to be reads. Any advice?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are only setting the table_id field in meter_write.table_entry. This is not enough, every table entry has its own meter entry. You need to provide the full match key, like you would if you were doing a MODIFY on a TableEntry.

You need to be able to call set_match_key here, for the direct case: the index is replaced by a match key.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so the direct_meter match key needs to match exactly with an existing, already installed match key?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a write operation, yes.
The direct meter entry can also be configured at the time of inserting the table entry, to avoid 2 RPCs: https://github.com/p4lang/p4runtime/blob/90553b90a12ead5c19700e7fef21164dea5b6d22/proto/p4/v1/p4runtime.proto#L191

Copy link
Collaborator Author

@fruffy fruffy Apr 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I got something to work.
There is one edge case that is tricky. I am guessing the direct meter always requires an entry in the table to be configured? For example, if you have this construct:

table meter_table {
    actions = {
        meter_assign;
    }
    key = {
        h.eth_hdr.dst_addr: exact;
    }
    size = 16384;
    default_action = meter_assign();
    meters = table_attached_meter;
}

where meter_assign is executed as default action, one might have no entry on the table. I am guessing this case is not supported? I tried abusing the API with is_default_action but it did not work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P4Runtime & PI support direct resources for the default table entry, but bmv2 does not.
When attempting this, an error should be returned and the simple_switch_grpc logs should read: [PI] bmv2 does not support direct meters for default entries.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that was quite helpful.

@fruffy fruffy added the p4tools Topics related to the P4Tools back end label Apr 10, 2023
@fruffy fruffy force-pushed the fruffy/meter branch 2 times, most recently from f047b70 to 2d0c2c2 Compare April 12, 2023 12:56
@fruffy fruffy marked this pull request as ready for review April 12, 2023 13:33
@fruffy fruffy force-pushed the fruffy/meter branch 3 times, most recently from a8a2603 to 3a681f0 Compare April 19, 2023 00:22
const Bmv2V1ModelMeterValue *Bmv2V1ModelMeterValue::evaluate(const Model &model) const {
const auto *evaluatedValue = model.evaluate(getInitialValue());
auto *evaluatedMeterValue = new Bmv2V1ModelMeterValue(evaluatedValue, isDirect);
const std::vector<ActionArg> evaluatedConditions;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is evaluatedConditions unused?


const Bmv2V1ModelRegisterValue *Bmv2V1ModelRegisterValue::evaluate(const Model &model) const {
const auto *evaluatedValue = model.evaluate(getInitialValue());
auto *evaluatedRegisterValue = new Bmv2V1ModelRegisterValue(evaluatedValue);
const std::vector<ActionArg> evaluatedConditions;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@@ -47,9 +47,12 @@ class PTF : public TF {
float currentCoverage) override;

private:
/// @returns the static preamble string used by inja to serialize the test preamble.
static std::string getPreamble();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still needed?

Copy link
Contributor

@pkotikal pkotikal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

@fruffy fruffy changed the title Implement meter support for the BMv2 V1model PTF test back end [P4Testgen] Implement meter support for the BMv2 V1model PTF test back end May 2, 2023
@fruffy fruffy merged commit 6444bfb into main May 2, 2023
@fruffy fruffy deleted the fruffy/meter branch May 2, 2023 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4tools Topics related to the P4Tools back end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants